-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add makezero linter #1520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add makezero linter #1520
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution. golangci-lint
uses a bunch of opinionated linters and I'm not sure if the idea was to ever enable all. I see no reason to not add linters that could help with these things without the motive being that everyone should always use them. See exhaustivestruct for example, I don't think this is usable in general for most projects.
I see no reason to not merge this PR so I will approve it. However, if you're interested in feedback, I would stop using the terminology size
since Go refers to this as capacity
both in the documentation (see here) and in the AST.
The make function takes a type, a length, and an optional capacity.
Just noticed that this panics when running against golangci-lint itself so I'll have to look into that before this is merged. |
I think that there might be existing linter/rule to check this one, as Goland is showing similar warning for such cases for me, it could be Goland's thingy only 🤔. Give me sometime to check on this. It's ok to have some overlapping between linters, but we should try not to confuse end users. Recently, there is one good question about differences between linters (e.g. deadcuode vs unused) in golangci-lint slack channel. |
My bad, I was confused with another check |
I made a small change to makezero that fixed parsing of golangci-lint itself, so I think this is in a state to be merged. |
This comment has been minimized.
This comment has been minimized.
I found the bug introduced in #1569 and left a comment there. @ashanbrown - Please consider fixing that. Thank you! EDIT: a fix is already in main branch. The bug will hopefully be fixed in whatever version comes after 1.34.0. |
Would love to hear some thoughts on this linter. In the interest of full disclosure, it was rejected by gocritic (see go-critic/go-critic#875). In practice, it has been immensely useful to my team. Thanks for your consideration. Details follow:
Makezero discourages the creation of slices with nonzero capacity but zero length. Its primary goal is to prevent appending to an initialized slice with nonzero length, but it can be used to disable uninitialized slices with nonzero length altogether. A common error among go programmers, particular when egged on by the useful
prealloc
linter, is this one:In this case, the programmer has forgotten to set the initial length of the array to 0 with
make([]int, 0, len(y)
. In practice, my company has found it useful to anymake
calls with nonzero length, because the need to optimize by avoidingappend
statements is limited. We also like seeingappend
statements because they discourage the additional indirection introduced by indices needed to access elements in a preallocated non-zero length array (i.e. we don't see the index variablei
all over the place like you'd see in C-language programs). We favorable readability of code over performance in almost all cases, addingnolint
directives when we favor performance.See https://github.com/ashanbrown/makezero for more details.